Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade phpcodesniffer-composer-installer #36791

Closed
wants to merge 14 commits into from

Conversation

fredden
Copy link
Member

@fredden fredden commented Jan 30, 2023

Description

dealerdirect/phpcodesniffer-composer-installer has now reached its first stable version. See https://github.com/PHPCSStandards/composer-installer/releases/tag/v1.0.0 for details.

Manual testing scenarios

There are no functional changes between these two versions.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Upgrade phpcodesniffer-composer-installer #36913: Upgrade phpcodesniffer-composer-installer

@m2-assistant
Copy link

m2-assistant bot commented Jan 30, 2023

Hi @fredden. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@m2-github-services m2-github-services added Partner: Fisheye partners-contribution Pull Request is created by Magento Partner labels Jan 30, 2023
@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie engcom-Charlie added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Jan 31, 2023
Copy link
Contributor

@Den4ik Den4ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fredden Thanks for your update.
Please review my comment

@@ -90,7 +90,7 @@
},
"require-dev": {
"allure-framework/allure-phpunit": "^2",
"dealerdirect/phpcodesniffer-composer-installer": "^0.7",
"dealerdirect/phpcodesniffer-composer-installer": "^0.7 || ^1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As same as for codding-standard repo PR I believe that "^1.0" will be enough

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing here that's not compatible with ^0.7.0. I've specifically kept this because I expect other packages may take a while to upgrade / allow the new version. If there's something that requires ^0.7, it'll work today, but not after merging if we change this to ^1.0 only. Listing both supported versions means that interoperability with other packages is maintained; Composer will pick the "best" version, which is typically the highest mutually-supported version. If we use ^1.0 only here, then we're forcing others to upgrade their packages in order for their coding standard to be used in conjunction with this product.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense

Copy link
Contributor

@Den4ik Den4ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed test is not related to changes

@glo30253
Copy link

@magento run Functional Tests B2B

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@glo30253
Copy link

glo30253 commented Feb 15, 2023

Hi @fredden,@Den4ik
Thanks for the collaboration & contribution!

✔️ QA Passed

Manual Scenario Steps
Before: ✖️
We have checked with 2.4-develop and phpcodesniffer was 0.7.2 version
Screenshot 2023-02-15 at 11 30 40 AM

After: ✔️
We have checked with PR changes and phpcodesniffer composer installer upgrading from 0.7.2 t0 1.0
Screenshot 2023-02-15 at 11 32 54 AM

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

As per above comment, moving it to Merge In Progress.

Thank you!

@engcom-Lima
Copy link
Contributor

@magento create issue

@engcom-Charlie
Copy link
Contributor

Hi @fredden,

Based on this comment, can we close this PR.

Thank you!

@fredden
Copy link
Member Author

fredden commented Sep 11, 2023

@engcom-Charlie, Adobe needs to decide if they want to accept the breaking changes introduced by @glo71317 in d337684 (in which case this pull request should be closed), or not (in which case this pull request should be merged as was the plan since 15th February 2023).

@glo71317
Copy link
Contributor

@engcom-Charlie, Adobe needs to decide if they want to accept the breaking changes introduced by @glo71317 in d337684 (in which case this pull request should be closed), or not (in which case this pull request should be merged as was the plan since 15th February 2023).

Hi @fredden, i don't think there is any breaking changes, And also not forcing to any other vendor and customer to update the dependency in current magento version which they are using they can continue without any breaking changes, As per my understanding If they will upgrade with current version to upcoming 2.4.7-beta2 they can do without any breaking changes.
If you are facing any issue please let me know what are the breaking changes introduced by d337684

@fredden
Copy link
Member Author

fredden commented Dec 27, 2023

please let me know what are the breaking changes introduced by d337684

I have already articulated this. Please see #36791 (comment)

That's a breaking change. (See discussion here: #36791 (comment).)

Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

@Den4ik
Copy link
Contributor

Den4ik commented Dec 27, 2023

@glo30253 I'm agree with @fredden
We can't guarantee that thouthand's of Magento modules will work with this update. in this case Magento upgrade could break the stores. Make sense support both versions in the next minor release and remove old version at the second minor release. It provide some time to developers for update.

Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

@glo71317
Copy link
Contributor

glo71317 commented Jan 25, 2024

@fredden @Den4ik As per discussion with PO and Engineering Manager.
We have to downgrade at "dealerdirect/phpcodesniffer-composer-installer": "^1.0” to "dealerdirect/phpcodesniffer-composer-installer": "^0.7”

@Den4ik please update the pr based on above discussion if you want to deliver these changes else we will do and cancel this PR?

@fredden
Copy link
Member Author

fredden commented Jan 25, 2024

@glo71317 you introduced the breaking change in d337684. This pull request fixes the breaking part of that commit by re-introducing support for versions 0.7.x of this package. Before your change, this pull request was already backwards-compatible. Please can you arrange for this to be merged, as was the intention back in February 2023. (See #36791 (comment))

@Den4ik
Copy link
Contributor

Den4ik commented Jan 25, 2024

@glo71317 @fredden provided enough comment

@glo71317
Copy link
Contributor

@fredden @Den4ik Okay make sense thanks for your contribution.
@Indraniks Can you proceed for merging this pr?

@Den4ik
Copy link
Contributor

Den4ik commented Jan 26, 2024

@glo71317 Great, thanks for conversation

@engcom-Charlie
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, WebAPI Tests

Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

@engcom-Charlie
Copy link
Contributor

engcom-Charlie commented Jan 29, 2024

As the this comment, and as the PR is already been tested prior hence moving this PR to Merge in Progress.

@engcom-Charlie
Copy link
Contributor

This PR got merged in a2519ca hence closing this now.

@hostep
Copy link
Contributor

hostep commented Feb 26, 2024

@engcom-Charlie: please link to the publicly visible repo, it should be this link: a2519ca

@fredden fredden deleted the phpcs-installer branch February 26, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Partner: Fisheye partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue] Upgrade phpcodesniffer-composer-installer
8 participants